Make sure derived table column names are handled correctly #15588
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
systay
removed
NeedsDescriptionUpdate
|
Is this aiming for the 19.0.2 release? |
systay
force-pushed
the
derived-table-column-names
branch
from
071dc8e to
9720b76
Compare
5 tasks
systay
force-pushed
the
derived-table-column-names
branch
4 times, most recently
from
a580ad4 to
983dabd
Compare
systay
requested review from
harshit-gangal,
frouioui,
GuptaManan100 and
arthurschreiber
as code owners
systay
force-pushed
the
derived-table-column-names
branch
3 times, most recently
from
a9ae7e2 to
b358743
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
systay
force-pushed
the
derived-table-column-names
branch
from
40c60de to
a55df3f
Compare
Comment on lines
721
to
723
| // create new table ID | ||
| tableID := semantics.SingleTableSet(len(ctx.SemTable.Tables)) | ||
| ctx.SemTable.Tables = append(ctx.SemTable.Tables, nil) |
There was a problem hiding this comment.
nit: more comments can be added here as adding nil to semtable tables is not clear. I understand the intention here but comments will help.
There was a problem hiding this comment.
Ideally we can add the DerivedTable here which is created below.
There was a problem hiding this comment.
the Derived table that we create here is of a very different type than what we store in the semantic state, and the info that we normally store there does not really make sense to store here. we just need an ID
harshit-gangal
approved these changes
Apr 29, 2024
Signed-off-by: Andres Taylor <[email protected]>
…n-names Signed-off-by: Andres Taylor <[email protected]>
|
@systay is there a user-reported issue for this fix that is still open and should be closed? |
no, this was found during our own testing |
This was referenced May 2, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
For some situations, during planning we create derived tables. When this happens, we sometimes get the column names wrong, which leads to queries that are not correct.
Example query, with bad plan:
bad plan produced:
{ "QueryType": "SELECT", "Original": "select id, id from user union select id, bar from user_extra order by 1", "Instructions": { "OperatorType": "Sort", "Variant": "Memory", "OrderBy": "(2|3) ASC", "ResultColumns": 2, "Inputs": [ { "OperatorType": "Distinct", "Collations": [ "(0:4)", "(1:4)", "(2:3)", "3" ], "Inputs": [ { "OperatorType": "Route", "Variant": "Scatter", "Keyspace": { "Name": "user", "Sharded": true }, "FieldQuery": "select dt.id, dt.id, id, weight_string(id), weight_string(dt.id) from (select id, id from `user` where 1 != 1 union select id, bar from user_extra where 1 != 1) as dt where 1 != 1", "Query": "select dt.id, dt.id, id, weight_string(id), weight_string(dt.id) from (select id, id from `user` union select id, bar from user_extra) as dt", "Table": "`user`, user_extra" } ] } ] }, "TablesUsed": [ "user.user", "user.user_extra" ] }As you can see from the plan, one of the queries being sent down to mysql looks like:
This query would fail in MySQL with an ambiguous column error for
dt.id.Since this change is quite involved, and these queries have never worked well, I suggest we don't backport this change set.
Related Issue(s)
Checklist
Deployment Notes